Skip to content

framework: split defect-scan into mechanical + semantic phases; promote split pipeline to default - #4

Merged
TheAmericanMaker merged 2 commits into
masterfrom
claude/split-defect-scan
May 4, 2026
Merged

TheAmericanMaker merged 2 commits into
masterfrom
claude/split-defect-scan

Conversation

@TheAmericanMaker

Copy link
Copy Markdown
Member

Summary

Splits the single-phase defect scan into two phases that run at different points in the workflow, and promotes the new split pipeline (pipeline-full-with-deep-audit) to the default.

Why split? Passes 3 (concurrency), 4 (security), and 5 (API contract violations) produce significantly better signal once contracts and protocols are documented — that is what defines "violation" for those passes. Running them blind, before contracts/protocols, yields more false positives and misses semantic bugs. Running everything after protocols, on the other hand, costs the cheap-bugs feedback loop that contracts and porting benefit from.

The split:

  • defect-scan-mechanical (after architecture, before contracts) — passes 1, 2, 6: logic and correctness, error handling, configuration hazards. Context-light; the bugs visible from local code reading.
  • defect-scan-semantic (after protocols, before porting) — passes 3, 4, 5. Reads the mechanical report so it does not re-flag, honors carry_forward entries routed to it, and pass 5 cites the contract or protocol entry it claims is violated.
  • The porting bundle's new "Defect Synthesis" section consolidates both reports into a single porting-oriented view, with branching instructions for single-scan / split-scan / no-scan pipelines.

Why default? The product positioning is curated, citation-rich findings (security review, port planning) — not casual exploration. Depth-first audit is the right baseline for that audience. The legacy pipeline-full-with-audit is preserved as a documented alternative for cheaper runs; no phase IDs were renamed and no pipeline file was removed, so projects already initialised against the old default see no change.

Commits

  • 392b212 framework: split defect-scan into mechanical (early) + semantic (late) phases
  • d432093 framework: promote pipeline-full-with-deep-audit to default

What changed

New

  • .codecarto/findings/defect-scan-mechanical/{SKILL.md,README.md} — thin wrappers reusing findings/defect-scan/passes/*.md for the mechanical subset.
  • .codecarto/findings/defect-scan-semantic/{SKILL.md,README.md} — same for the semantic subset; declares contracts/protocols/mechanical-report as required reads.
  • .codecarto/templates/mechanical-defects.md — passes 1/2/6 + a "Routed To Semantic Phase" handoff table.
  • .codecarto/templates/semantic-defects.md — passes 3/4/5 + a "Carry-Forward Closure" table; pass 5 row includes a Spec Reference column.
  • .codecarto/workflow/pipeline-full-with-deep-audit.yaml — 7-phase pipeline. Mechanical depends on architecture; semantic depends on architecture + contracts + protocols + mechanical; porting and reimplementation-spec read both reports.

Modified

  • .codecarto/templates/reverse-engineering-bundle.md — adds a "Defect Synthesis" section + bumps the porting validation table to grade it without breaking existing pipelines.
  • .codecarto/workflow/status.yaml — pipeline pointer now pipeline-full-with-deep-audit.yaml; phases block updated to the 7-phase order; pipeline-switching comment lists all six current variants.
  • .codecarto/README.md, .codecarto/GUIDE.md, top-level README.md — pipeline-choice tables, walkthroughs, init alias list, folder layout, and the model-tier row all promote deep-audit-as-default and document the legacy single-scan as an opt-in.
  • extensions/codecarto/index.ts — adds full-with-deep-audit to PIPELINE_ALIASES; hardcoded /codecarto-init fallback updated to the new default. Legacy full-with-audit alias preserved.

Untouched (intentional)

  • .codecarto/findings/defect-scan/ (SKILL, README, all six passes/*.md)
  • .codecarto/templates/defect-report.md
  • All other pipeline files (pipeline-full-with-audit.yaml, pipeline.yaml, pipeline-defect-scan.yaml, pipeline-lite.yaml, pipeline-architecture-only.yaml)

Test plan

Verified during development via two sandbox scripts that exercise Pi's actual parseSimpleYaml/stringifySimpleYaml against the new files:

  • New pipeline parses cleanly through Pi's YAML parser
  • phase_order matches phases[].id exactly
  • Every depends_on references a real phase; the dependency DAG walks cleanly through phase_order
  • Every required_read either is GUIDE.md/workflow/status.yaml or is produced by an upstream phase
  • mechanical-defects.md and semantic-defects.md are both listed in porting's required_reads
  • pipeline_full-with-deep-audit round-trips through stringifySimpleYaml without losing phase_order
  • .codecarto/workflow/status.yaml's phases keys exactly match the default pipeline's phase_order
  • Every skill_path and output_template referenced by the default pipeline exists on disk
  • Pi's hardcoded fallback in /codecarto-init matches the packaged status.yaml's pipeline pointer
  • PIPELINE_ALIASES retains both the new full-with-deep-audit and legacy full-with-audit entries
  • status.yaml round-trips through Pi's parser without dropping any phase's carry_forward

Reviewer test (manual):

  • In a fresh repo, /codecarto-init (no args) and confirm the resulting .codecarto/workflow/status.yaml shows pipeline: workflow/pipeline-full-with-deep-audit.yaml and the 7-phase block.
  • In the same repo, /codecarto-init full-with-audit and confirm the legacy 6-phase pipeline is selected with the defect-scan phase block.
  • Run /codecarto-next from the fresh-default state and confirm the queued prompt is for architecture (no phase ordering regressions).

Generated by Claude Code

claude added 2 commits May 4, 2026 14:27
…) phases

Adds a new pipeline variant `pipeline-full-with-deep-audit` that splits the
single defect-scan phase into two passes, run at different points in the
workflow:

- defect-scan-mechanical (after architecture, before contracts) — passes
  1, 2, 6 from the legacy SKILL: logic and correctness, error handling,
  configuration hazards. Runs context-light; the bugs visible from local
  code reading without contracts/protocols context.
- defect-scan-semantic (after protocols, before porting) — passes 3, 4,
  5: concurrency and resources, security and trust, API contract
  violations. Runs with full context; pass 5 cites the contract or
  protocol entry it claims is violated.

Rationale: passes 3/4/5 produce many more false positives and miss
semantic bugs when run before contracts/protocols are documented. The
late-context pass yields better signal; the early mechanical pass keeps
the cheap-bugs feedback loop for the contracts and porting phases. The
porting bundle's new "Defect Synthesis" section consolidates both reports
into a single porting-oriented view.

Files:
- .codecarto/findings/defect-scan-mechanical/{SKILL.md,README.md} (new):
  thin wrappers that reuse findings/defect-scan/passes/*.md and document
  the subset + how to route semantic-flavored sightings forward via
  carry_forward.
- .codecarto/findings/defect-scan-semantic/{SKILL.md,README.md} (new):
  reads mechanical-defects.md so it does not re-flag, and honors
  carry_forward entries targeted at it.
- .codecarto/templates/mechanical-defects.md (new): subset template,
  passes 1/2/6 + a "Routed To Semantic Phase" handoff table.
- .codecarto/templates/semantic-defects.md (new): subset template,
  passes 3/4/5 + a "Carry-Forward Closure" table; pass 5 row includes a
  Spec Reference column.
- .codecarto/templates/reverse-engineering-bundle.md (modified): adds a
  "Defect Synthesis" section with pull-from instructions for single-scan
  vs split-scan vs no-scan pipelines, and bumps the porting validation
  table to grade the new section without breaking existing pipelines.
- .codecarto/workflow/pipeline-full-with-deep-audit.yaml (new): 7-phase
  pipeline. Mechanical depends on architecture; semantic depends on
  architecture + contracts + protocols + mechanical; porting and
  reimplementation-spec read both reports.
- extensions/codecarto/index.ts (modified): adds `full-with-deep-audit`
  to PIPELINE_ALIASES so /codecarto-init full-with-deep-audit works.

Existing pipelines (full-with-audit, full, defect-scan, lite,
architecture-only) are unchanged. The legacy `defect-scan` phase + SKILL
+ template + passes/ directory are untouched and still drive every
existing pipeline. Users opt into the split via /codecarto-init
full-with-deep-audit.

Verified: new pipeline parses through Pi's YAML parser; phase_order
matches phases ids; every depends_on references a real phase; DAG walks
cleanly; every required_read either is GUIDE.md/status.yaml or is
produced by an upstream phase; round-trip through stringifySimpleYaml
preserves phase_order.
Make the depth-first split-defect-scan pipeline the default everywhere
the old `pipeline-full-with-audit.yaml` was named as such, on the
positioning that CodeCartographer's edge is curated, citation-rich
findings (security review, port planning) — not casual exploration. Deep
audit is the right baseline for that audience.

- .codecarto/workflow/status.yaml: pipeline pointer now
  workflow/pipeline-full-with-deep-audit.yaml. phases block updated to
  the new 7-phase order (defect-scan replaced by defect-scan-mechanical
  in position 2 and defect-scan-semantic in position 5). The pipeline-
  switching comment reflects all six current variants.
- .codecarto/README.md: Choosing a Pipeline lists deep-audit first and
  marked as default, with the legacy single-scan, the no-scan, and the
  scaled-back variants as opt-ins.
- .codecarto/GUIDE.md: Pipeline Selection table gains the deep-audit row
  (now default) above the legacy full-with-audit row; folder-layout
  block lists the new findings/defect-scan-{mechanical,semantic}/ dirs
  and surfaces both pipeline files.
- README.md (top-level): pipeline-choice block, /codecarto-init alias
  list, Pipeline Variants table, and the Frontier model-tier row all
  promote deep-audit and keep the legacy pipeline as a documented
  alternative for cheaper runs.
- extensions/codecarto/index.ts: hardcoded codecarto-init fallback
  changed from pipeline-full-with-audit.yaml to
  pipeline-full-with-deep-audit.yaml. PIPELINE_ALIASES still includes
  both names so existing scripts pinning full-with-audit keep working.

No phase IDs were renamed and no existing pipeline file was removed —
projects already initialised against full-with-audit see no change.

Verified: status.yaml's phases block matches the new default pipeline's
phase_order exactly; every skill_path and output_template referenced by
the default pipeline exists on disk; status.yaml round-trips through
Pi's parser/stringifier without losing carry_forward; the new
hardcoded fallback in codecarto-init points at the same file as the
packaged status.yaml.
@TheAmericanMaker
TheAmericanMaker merged commit c99762f into master May 4, 2026
@TheAmericanMaker
TheAmericanMaker deleted the claude/split-defect-scan branch May 4, 2026 15:43
TheAmericanMaker pushed a commit that referenced this pull request May 4, 2026
…line

Post-merge follow-ups missed by PR #4. None changes behavior of the
existing pipelines; all three are docstring/comment fixes that bring
the SKILL/template surface area in line with the new default.

- findings/porting/SKILL.md:8 — porting agents in the new default
  pipeline were instructed to read findings/defect-scan/defect-report.md
  (which doesn't exist in deep-audit runs). Now mentions both shapes:
  the legacy single report AND the mechanical+semantic pair.
- templates/behavioral-contracts.md:121 — the comment listing which
  pipelines the default 6-criterion validation table matches now
  includes full-with-deep-audit (whose contracts phase has the same
  6 criteria as the legacy ones).
- findings/defect-scan/SKILL.md:62 — the action-set parenthetical
  ("Pre-porting pipelines") now lists both full-with-audit and
  full-with-deep-audit, since the new SKILLs point users back here
  for the action classification scheme.

Found via doc-sync skill scan after PR #4 merged.
TheAmericanMaker added a commit that referenced this pull request Sep 28, 2026
…mtime, read HEAD through a worktree's gitdir

The second review of #445 found three medium findings and one gap the
pilot would hit on day one; each is reproduced by its probe before the fix
and pinned by a test that kills the corresponding mutant.

Re-read AGAIN before minting (review #4). The gate and the tree re-read
ran once, before the request; nothing re-checked after the person
answered, so a tree edited while the form was open still minted an
approval bound to a candidate the tree no longer matched. requestAcceptance
now runs args.reread again after elicitationDecision says accepted and
checks it with checkCandidateFreshness; a mismatch or a read failure is
`refused` with a reason starting `proof-stale:`, the stored request is
kept, nothing is minted, and the marker is released. This is the contract's
"before issuing AND AGAIN BEFORE MINTING".

Unparseable in-flight marker (review #5). JSON.parse on an empty or
truncated marker threw into the outer catch, which set stale=false, so the
mtime fallback the comment promised was unreachable and the attempt was
wedged forever. The parse is now its own try; an unparseable marker (or
one without a finite expires_at) is judged by lstat().mtimeMs against the
contract's maximum request TTL. Only a regular file is ever cleared: a
directory or a symlink at the marker's path is refused by name with
invalid-state and never removed (no rm -r).

Worktree layout (review, recommended). A `.git` FILE (`gitdir: <path>`)
failed closed with "no HEAD could be read". readWorkingTree now resolves
the gitdir line (absolute or relative to the worktree root), reads HEAD
from that git dir, and resolves a symbolic ref through the per-worktree
dir first and then the `commondir` (relative to the gitdir) for shared
loose refs and packed-refs. Anything malformed still refuses with the same
reason; nothing outside what gitdir:/commondir name is followed; no git
process is spawned (the module imports no child_process).

Guards for the five mutants that slipped past (review #8): core-level
calls with reread undefined and with a failing reread both stop at
needs-human-acceptance with nothing presented; real-file tests pin the
executable bit, symlinks recorded as links and never followed, the
candidate's own exclusions, and unreadable files reported as uncovered.

Refs #409.
TheAmericanMaker added a commit that referenced this pull request Sep 28, 2026
…on) (#445)

* feat: MCP acceptance adapter — request_acceptance asks a person through elicitation

Adds core/engineering/acceptance.ts and a `request_acceptance` action on
codecarto_change. HostCapabilities are derived from the session (client
capabilities + clientInfo at initialize) and host config, never from request
fields; acceptanceChannelSupported decides whether the client may be asked
(registry still empty, so every host returns needs-human-acceptance today);
the decision comes only from the client's elicitation response via
elicitationDecision; an approval is minted only for `accepted`, checked with
evaluateApprovalReceipt against the stored request before persisting, and
returned with classifyAcceptance's reading (cooperative on this host, D3).

The SUPPORTED path is tested through an @internal buildServer option that
only tests/helpers/acceptance-server.mjs passes; bin.mjs never does.

Refs #409

* fix: run the gate on a fresh tree re-read before asking, and mint at most one approval per candidate

requestAcceptance never ran the acceptance gate and never re-read the tree,
so it asked a person about — and minted approvals for — candidates the gate
refuses (no review at all) and bytes that had moved since capture. It also
minted a second accepted approval for one candidate, sequentially or
concurrently. Review of #445 (Refs #409).

- core/engineering/acceptance.ts: before any request is built, the caller's
  `reread` of the working tree is passed to evaluateAcceptanceGate as
  candidate_reread; unless the gate says may-accept, no request is issued
  and nobody is asked (`blocked` with the blockers via describeGateOutcome;
  `needs-human-acceptance` when the tree cannot be re-read). An accepted
  approval already bound to this candidate's id+digest refuses with
  invalid-state/already-accepted. Request→present→mint is serialized per
  attempt by an O_EXCL marker `requests/.in-flight` carrying the request's
  expires_at; a marker past it is cleared. The store's change lock is not
  held across the elicitation wait.
- mcp-server/working-tree.ts (new): re-reads cwd in the candidate's scope
  (same exclusions, `.git/` skipped, HEAD read from .git/HEAD; no git
  process). Both `gate` and `request_acceptance` supply it.
- mcp-server/server.ts: buildServer() is the only public overload; the
  options overload is @internal, so the published .d.ts no longer names a
  stripped interface (TS2304 under skipLibCheck: false).
- tests: gate-refused, edited-tree, gate-action-on-edited-tree, sequential
  and concurrent duplicate, stale marker, forged elicitation content (M6),
  and a consumer compile of dist with the repo's tsc and skipLibCheck off.
  acceptance moves to layer 5 in the layer map (it now imports gates.ts).
- docs: registry entry shape after a live check; the assurance wording wart.

* test: declaration check resolves dist on Windows (#445)

The new "published declarations compile" test failed on test-windows
with TS2307. It imported dist through pathToFileURL(...).pathname, which
on Windows is "/D:/a/...", a path tsc cannot resolve.

The consumer now lives in a scratch directory under node_modules/.cache
and imports dist by a relative specifier. A relative path from the OS
temp dir does not work either, because the runner's temp dir is on C:
and the checkout is on D:. The scratch directory is removed afterwards.

Checked that it still catches the bug it exists for: putting back the
dangling BuildServerOptions reference in dist fails the test with
TS2304.

* fix: re-check freshness before minting, clear unparseable markers by mtime, read HEAD through a worktree's gitdir

The second review of #445 found three medium findings and one gap the
pilot would hit on day one; each is reproduced by its probe before the fix
and pinned by a test that kills the corresponding mutant.

Re-read AGAIN before minting (review #4). The gate and the tree re-read
ran once, before the request; nothing re-checked after the person
answered, so a tree edited while the form was open still minted an
approval bound to a candidate the tree no longer matched. requestAcceptance
now runs args.reread again after elicitationDecision says accepted and
checks it with checkCandidateFreshness; a mismatch or a read failure is
`refused` with a reason starting `proof-stale:`, the stored request is
kept, nothing is minted, and the marker is released. This is the contract's
"before issuing AND AGAIN BEFORE MINTING".

Unparseable in-flight marker (review #5). JSON.parse on an empty or
truncated marker threw into the outer catch, which set stale=false, so the
mtime fallback the comment promised was unreachable and the attempt was
wedged forever. The parse is now its own try; an unparseable marker (or
one without a finite expires_at) is judged by lstat().mtimeMs against the
contract's maximum request TTL. Only a regular file is ever cleared: a
directory or a symlink at the marker's path is refused by name with
invalid-state and never removed (no rm -r).

Worktree layout (review, recommended). A `.git` FILE (`gitdir: <path>`)
failed closed with "no HEAD could be read". readWorkingTree now resolves
the gitdir line (absolute or relative to the worktree root), reads HEAD
from that git dir, and resolves a symbolic ref through the per-worktree
dir first and then the `commondir` (relative to the gitdir) for shared
loose refs and packed-refs. Anything malformed still refuses with the same
reason; nothing outside what gitdir:/commondir name is followed; no git
process is spawned (the module imports no child_process).

Guards for the five mutants that slipped past (review #8): core-level
calls with reread undefined and with a failing reread both stop at
needs-human-acceptance with nothing presented; real-file tests pin the
executable bit, symlinks recorded as links and never followed, the
candidate's own exclusions, and unreadable files reported as uncovered.

Refs #409.

* test: the worktree fixture is isolated from the runner's git config (#445)

test (t) failed only on test-windows. After switching the worktree back
to `feature` it expected FRESH and got STALE.

The fixture passed GIT_CONFIG_GLOBAL=/dev/null and GIT_CONFIG_NOSYSTEM to
the main-checkout calls only. The two checkouts inside the worktree ran
under the runner's own config. On Windows that includes system
core.autocrlf=true, so a.txt was rewritten with CRLF. The byte-exact
re-read rightly called the tree STALE. /dev/null is not a path on
Windows either.

Reproduced on Linux with GIT_CONFIG_SYSTEM pointing at a file setting
core.autocrlf=true: the original test fails at the same line (902) and
the fixed test passes.

The file now imports tests/helpers/git-environment-isolation.mjs first,
as every other test that builds a git fixture does. It is added to that
helper's guarded list (seven files, now eight), so the "imported first"
check covers it. Every git call in the fixture uses one environment.
This change is to the test only. readWorkingTree and its CRLF handling
are unchanged, and byte-exactness is intended.

npm test 1357/1357 twice.

* test: pin the worktree HEAD reader's refusals (#445)

The third review left four guard branches in mcp-server/working-tree.ts
that no test covered. A mutant that removed any one of them still passed
33/33:
- gitdir must be a real directory
- the sha must be exactly 40 lowercase hex
- '..', leading '/' and backslashes are refused in a ref
- empty segments are refused in a ref

Test (u) builds each malformed HEAD, ref or gitdir. Each refused case
also plants a file the reader could resolve to a valid sha if its guard
were missing, and each has a well-formed twin that is accepted. All
four mutants now fail the test, as does a mutant that drops only the
backslash check. The test is POSIX only (symlinks and backslash file
names).

npm test 1358/1358 twice.

* test: replace, not overwrite, the worktree .git file in (t) (#445)

On test-windows (runs for 14a8887 and 9438a73), the malformed-.git
loop in (t) fails with EPERM when it opens <tmp>/wt/.git. Git for
Windows marks a worktree's .git file hidden (core.hideDotFiles defaults
to dotGitOnly), and overwriting a hidden file in place is EPERM on
Windows. The loop now removes the file before writing it. The
autocrlf fix from 14a8887 held: the failure moved past the FRESH
assertion at line 902.

Only the test changes; readWorkingTree is untouched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants