fix(sensors): stop reporting harness failures as code-quality FAILs

[orossant]

Problem

On a real deployment, the advisory linter and type-check sensors reported FAIL for every file on an infrastructure-design stage — a stage that writes methodology artifacts to Neptune and no code to disk. Each file entry looked like:

{ "file": "packages/api/vitest.config.ts", "result": "FAIL", "detail": null, "timedOut": false }

~40 files, all FAIL, detail: null. A 100% failure rate across config and test-setup files in two different packages is not 40 independent genuine findings. Three distinct defects combined to produce it.

1. Sensors were graded on files the stage never touched

runScriptSensor globbed the whole checkout, so a design stage matched .ts files left behind by an earlier code-generation stage and spawned the type-checker once per file. The stage was judged on code it never wrote.

2. Exit 127 (tool unresolvable) mapped to FAIL

The per-sensor scripts probe their tool at startup and exit 127 on any non-zero probe — bunx <tool> returns assorted non-127 codes for network-fetch / package-resolution / registry-timeout failures, so the scripts normalise to 127 themselves. Upstream a dispatcher branch reclassified that to a tool-unavailable note; the reclassification was dropped when the dispatcher was reimplemented as resultFromExit. The result enum's own comment reserves INCONCLUSIVE for exactly this case ("tool unavailable"), so this was a straightforward miss.

3. Any non-zero exit mapped to FAIL, and stderr was discarded

bun/node sensors carry their verdict in stdout JSON at exit 0, so a genuine lint/type defect always exits 0 with {"pass": false}. Any non-zero exit is therefore a script/tool error, never a code verdict. Worse, runChild captured stderr and then threw it away — resultFromScript returned only {result, detail} — so these failures were undiagnosable from the persisted SensorRun row once the container was gone.

Change

• Scope script sensors to the files the stage changed, using the list the git engine already captures in commitAll (status --porcelain before staging). null = unknown provenance, inspect everything (never silently skip a check); [] = nothing changed on disk. Matching is suffix-tolerant because the engine reports repo-relative paths while listFiles reports workspace-relative ones, which differ on a multi-repo checkout.
• 127 maps to INCONCLUSIVE in resultFromExit, via a named TOOL_UNAVAILABLE_EXIT constant.
• Non-zero exit with no stdout verdict maps to INCONCLUSIVE (reason: "script-error") for bun/node runtimes. sh keeps the exit-code convention, since a shell sensor legitimately signals failure that way.
• Retain exitCode and a tail-truncated stderr on every non-PASS file entry. Truncated from the tail because the interpreter's real error is the last thing written, and budgeted at 500 chars per file so a wide glob cannot approach the DynamoDB item ceiling.
• Fix the worst-result aggregation: with the new mapping an all-INCONCLUSIVE fan-out would have aggregated to PASS — a false green strictly worse than the original FAIL.

Testing

• sensor-runner.test.js, run-stage.test.js, v2-sensor-contract.test.js: 197 passed
• New tests cover: 127 to tool-unavailable, exits 1/3/66 to script-error, sh exit 1 still FAIL, null exit still BLOCKED, tail truncation preserving the real error, suffix vs partial-segment matching, and an end-to-end test reproducing the exact production shape (every file exits non-zero with no stdout, yielding INCONCLUSIVE rather than FAIL)
• A companion test asserts a real defect (exit 0 with pass:false) is still reported as FAIL, so genuine findings are not masked
• oxlint clean, oxfmt --check clean

Verified live after deploying to a dev environment:

linter     -> INCONCLUSIVE  {"reason":"no changed files match","globbed":55,"changed":0}
type-check -> INCONCLUSIVE  {"reason":"no changed files match","globbed":68,"changed":0}

68 files globbed, 0 changed — previously all 68 were spawned against and reported FAIL. And on a stage that did change code, the 127 path fired for real and is now diagnosable from the row alone:

linter     -> INCONCLUSIVE  {"reason":"tool-unavailable","exitCode":"127","stderr":"eslint-unavailable"}
type-check -> INCONCLUSIVE  {"reason":"tool-unavailable","exitCode":"127","stderr":"tsc-unavailable"}

Follow-up (not in this PR)

Those 127s are honest, but they do mean the code-quality axis is currently inert on code stages: bunx eslint and bunx tsc cannot resolve without the checkout's dependencies installed, which the runtime deliberately avoids for inode-budget reasons. Worth a separate discussion — type-check arguably belongs in CI on the pushed branch (tsc genuinely needs installed types to be meaningful), while linter could move to a zero-dependency single binary such as oxlint, which this repo already depends on.

Notes

Rebased onto latest main (b9ab0b6).

I confirm the licensing of this contribution under the repository's MIT-0 license.

[JWThewes]

Thanks for the PR. The overall direction makes sense, and the added diagnostics are useful. I left some comments on cases that can still produce false results or lose the sensor record. Please address those before merging.

[JWThewes]

Could we avoid applying this file-level scope to type-check? That sensor runs tsc --project for the whole project, then filters diagnostics back to the supplied file. If a changed provider breaks an untouched consumer, only running the provider returns PASS because the error is attributed to the consumer. I reproduced that with the real Bun/TypeScript fixture. A config-only change has the same problem because no changed TS file reaches the sensor. Please keep linting file-scoped, but run type-checking once per affected project (or expand to all files in the affected project).

[JWThewes]

This suffix match loses repo identity. In a multi-repo workspace, if repo A and repo B both have src/index.ts, changing it in A selects both files and can grade the stage on untouched code from B. The changed-file list should carry normalized workspace-relative paths including the repo prefix, then use exact matching. While doing that, please consume NUL-delimited porcelain output so quoted filenames (spaces/non-ASCII) match listFiles correctly.

[JWThewes]

This assumes every Bun/Node sensor uses the stdout-JSON verdict contract, but the sensor model/editor allows custom scripts and the existing fallback treats exit 1 as FAIL. Those sensors would now be reported as INCONCLUSIVE based only on runtime. Could we key this behavior off an explicit verdict mode/output contract (or, minimally, the known linter/type-check sensor IDs) and preserve exit-code semantics for unmarked sensors?

[JWThewes]

stderr and exitCode are already present inside detail for script errors, so this duplicates the diagnostic for every file. More importantly, a per-file cap does not enforce DynamoDB’s 400 KiB item limit: with this exact shape, 354 failures with 500-character stderr produce about 410 KB before row metadata, and recordSensorRun then fails silently. Please add a total serialized-size budget (ideally dedupe identical harness failures into one sensor-level diagnostic plus counts/sample files) instead of only truncating each entry.

[orossant]

Thanks — all four are fair, and two of them are cases where my change makes things worse rather than just incomplete. Taking them in order of severity:

type-check file scoping produces false greens. This is the one that matters most. Since tsc --project checks the whole project and only filters diagnostics back to the supplied file, scoping to changed files means a changed provider breaking an untouched consumer reports PASS — the error is attributed to the consumer, which isn't in the changed set. A config-only change reports nothing at all. That's strictly worse than the spurious FAIL this PR set out to fix, and the known cross-file limitation documented in the upstream script header should have led me there. I'll keep lint file-scoped and run type-check once per affected project.

Suffix matching loses repo identity. I chose it deliberately, reasoning that including a borderline file was safer than skipping one, but you're right that in a multi-repo workspace it can grade a stage on a different repo's untouched code. I'll normalize the changed-file list to repo-prefixed workspace-relative paths and match exactly, and switch to NUL-delimited porcelain so filenames with spaces or non-ASCII survive the round trip.

Runtime is the wrong key for the verdict contract. Agreed — keying "non-zero exit means script error" off bun/node silently reclassifies any custom script that legitimately signals FAIL via exit code. I'll drive it from an explicit verdict mode rather than the runtime, falling back to the existing exit-code semantics for sensors that don't declare one.

The stderr budget doesn't bound the item. Your arithmetic is right and my per-file cap doesn't address the real constraint — and a silent recordSensorRun failure loses the entire verdict, which is worse than the missing diagnostics I was trying to add. I'll dedupe identical harness failures into a single sensor-level diagnostic with counts and sample files, under a total serialized-size budget, and drop the per-file duplication of fields already present in detail.

Will push a revision. I'm doing #363 first since it carries a credential-exposure path, then this one.

[orossant]

Revision pushed (8444437). All four addressed.

1. type-check no longer file-scoped. Scoping is now per sensor scope, declared on the sensor (file | project), rather than applied uniformly. The linter stays file-scoped; type-check widens to every matching file in each affected project. A change to the project config alone widens too, which covers the config-only case you raised. The end-to-end test asserts an untouched consumer is inspected while an unaffected project is not — and I confirmed it fails if the widening is disabled, since the helper's own unit tests would have passed either way.

2. Exact matching on workspace-relative paths. commitAndPushAll now re-bases each repo's paths onto the workspace root (toWorkspaceRelative), so src/index.ts in two repos are two distinct paths, and the runner matches exactly. Also switched to git status --porcelain -z: the NUL form is unquoted, so spaces and non-ASCII survive and can match listFiles. Rename entries drop the origin field and keep the destination. Covered by unit tests plus a real-git case committing a file.ts and café.ts.

3. Verdict contract is declared, not inferred. Added verdictMode (stdout-json | exit-code) resolved by sensorVerdictMode. Unmarked sensors default to exit-code, so custom scripts keep exit-status semantics; only sensors declaring the stdout-JSON contract get the script-error reclassification. The known linter/type-check ids fall back to stdout-json for rows predating the field. 127 stays INCONCLUSIVE in both modes.

4. Total size budget with dedupe. Identical harness failures collapse into one harnessFailures entry carrying fileCount and up to 5 sampleFiles; genuine per-file verdicts are kept. The per-file exitCode/stderr duplication is gone — those already live in detail. The whole detail is then trimmed to a serialized byte budget, recording filesOmitted. A 4000-entry test asserts the result stays inside the budget, since as you noted a silent recordSensorRun failure loses the entire verdict, which is worse than losing diagnostics.

307 tests pass across sensor-runner, git-engine, lane, resolve-conflict, run-stage and v2-sensor-contract; oxlint and oxfmt clean. Each new test was verified load-bearing by reverting its corresponding change rather than assumed.

Note lambda/agentcore/test/integration.test.js fails in my environment for want of the DynamoDB Local container — I checked it fails identically on main, so it is unrelated to this change.

[JWThewes]

Thanks for the revision. The main cases from the first review are in much better shape: ordinary project widening, exact workspace paths, and default exit-code behavior are covered. I found four remaining paths that can still skip a check or lose the SensorRun, so I think these need addressing before merge.

[JWThewes]

Could we make the size cap cover harnessFailures as well? The loop only removes detail.files, so if stderr differs per file (for example, because it includes the path), every failure becomes its own group and this array is never trimmed. I reproduced this with 1,000 distinct failures and the detail serialized to 622,801 bytes, above the DynamoDB 400 KiB limit; recordSensorRun would still lose the entire verdict. Please budget both arrays (or build the summary incrementally within the limit) and add a regression test with many distinct harness failures.

[JWThewes]

Could we account for paths that no longer exist? A deleted .ts file is absent from globbed, so this branch never marks its project as affected; deleting a provider that breaks untouched imports therefore skips type-checking. Deleting the project config has the same problem because its root is no longer in projectRoots, and dropping the old side of a rename misses moves out of or between projects. Please derive affected roots from the changed/status paths before requiring the path to exist, preserve the old rename path for project-scoped checks, and cover source deletion, config deletion, and a cross-project rename in tests.

[JWThewes]

There is a retry gap in this provenance calculation. If the first run commits successfully but push fails, runStage returns before sensors. On a retry with no further edits, commitAll reports clean, commitAndPushAll pushes the already-ahead commit, and this expression produces []; advisory script sensors then skip the commit that has never been checked. Please carry the file list for the ahead commit (or derive it from the remote-to-HEAD diff) and add a push-failure followed by clean successful retry test.

[JWThewes]

One wiring gap: an explicit declaration does not reach this helper in a real run. resolveSensors in v2-execution-plan.js copies a fixed set of fields and drops verdictMode, scope, and projectConfig, so a custom stdout-json sensor is still evaluated as exit-code; the helper test passes because it calls this function directly. Please thread these fields through block mapping and plan resolution, and add a plan-level test asserting that the resolved sensor retains them.

[orossant]

Revision pushed (acbbfab). All four addressed.

1. Deleted files mark the owning project as affected. expandToAffectedProjects now derives project roots from changed paths even when those paths are absent from the glob (deleted source, deleted config, cross-project rename). A deleted config's root is added to allRoots so projectRootOf can still resolve files under it. Three new tests cover source deletion, config deletion, and a cross-project rename — each verified load-bearing by reverting the corresponding change.

2. Retry provenance gap closed. New aheadFiles() in git-engine.js derives the changed-file list from git diff --name-only -z origin/<branch> HEAD when commitAll returns clean but the repo is ahead. commitAndPushAll calls it before pushing, so sensors always receive the file list for a never-checked commit. Two new tests: one for aheadFiles directly, one end-to-end proving the retry path carries files.

3. harnessFailures array is now bounded. summarizeFileResults trims harnessFailures (not just detail.files) when distinct stderr per file prevents dedup. Regression test with 1,000 distinct failures asserts the result stays inside DETAIL_BUDGET_BYTES.

4. resolveSensors threads verdictMode, scope, and projectConfig. Added the three fields to the mapped object in v2-execution-plan.js. Plan-level test asserts a custom stdout-json sensor retains all three after resolution.

223 tests pass (sensor-runner, git-engine, v2-sensor-contract, v2-execution-plan); oxlint and oxfmt clean.

[JWThewes]

Thanks for the second round — the harnessFailures budget, the deleted-file project expansion, and the resolveSensors threading all look good now, including the regression tests I asked for. While re-checking I found one regression this commit introduces and a few follow-ups:

1. Double workspace prefix on the normal multi-repo commit path (blocker). commitAndPushAll now applies toWorkspaceRelative twice: once right after commitAll (the block from the first commit) and again after the new aheadFiles fallback. On the ordinary path — commit succeeded, multi: true — commit.files is already prefixed when it hits the second block, so it comes out as owner/repo/owner/repo/src/index.ts. The exact-match scoping then selects nothing, the sensor reports no changed files match → INCONCLUSIVE, and since severityGate treats INCONCLUSIVE as non-PASS, a blocking script sensor holds every multi-repo code stage. The multi-repo test only asserts committed/pushed, never files, which is why it slips through. Could we apply the prefix exactly once (keep the one after the fallback, drop the earlier block) and extend the multi-repo test to assert the files shape?

2. The rename fix only exists in the unit test. expandToAffectedProjects now handles the old side of a rename correctly when it appears in changed, but neither producer supplies it: parsePorcelainZ explicitly drops the origin field for R/C entries, and aheadFiles runs git diff --name-only without --no-renames, so a detected rename surfaces only under its new name. Unstaged renames happen to show up as delete+untracked (covered), but a rename the agent already staged with git add, or one crossing the aheadFiles diff, loses the source project — the exact case from my earlier comment. Emitting the origin path from parsePorcelainZ and adding --no-renames to the aheadFiles diff should close it; please also cover the staged-rename case end to end.

3. Retry provenance, never-pushed branch. isAheadOfRemote treats a missing refs/remotes/origin/<branch> as ahead, but aheadFiles diffs against that same missing ref, exits non-zero, and returns []. So on a clean retry after the first-ever push of a stage branch failed, sensors still skip the never-checked commit — the same gap as before, in a narrower case. A fallback (merge-base against the default branch, or git show --name-only over the unpushed commits) would cover it.

4. stdout-json with a missing verdict silently PASSes. In resultFromScript, a stdout-json sensor that exits 0 with empty or unparseable stdout (or JSON without a boolean pass) falls through to resultFromExit(0) → PASS. Under the declared contract, a verdict-less exit 0 is a script defect and should be INCONCLUSIVE — otherwise a broken output format turns a blocking sensor into always-pass.

Item 1 is a must-fix for me since it breaks the multi-repo scoping this PR set out to get right; 2 was the point of my earlier comment so I'd like it in this PR as well. 3 and 4 I'm fine tracking as follow-ups if you prefer.